-
Notifications
You must be signed in to change notification settings - Fork 120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add support for google analytics userId #15
Conversation
looks good from my side. 👍 for merging |
please no version bumping in PRs |
|
||
tracker_options | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this maybe more generic? always camelize the keys, support #call
on every option....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point and useful for the cookie_domain with an i18n multihost setup 17000d3
@@ -93,6 +121,15 @@ def env | |||
end | |||
end | |||
|
|||
describe "with user_id tracking" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't that something that should only be tested in the integration spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the cookieDomain thingy is tested here too.
both can be tested again in the integration spec too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i'm just a bit confused as to what i should test where
* remove one if * refactor spec
@@ -28,6 +31,18 @@ def tracker | |||
options[:tracker].respond_to?(:call) ? options[:tracker].call(env) : options[:tracker] | |||
end | |||
|
|||
def tracker_options | |||
@tracker_options ||= begin | |||
tracker_options = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i know its a matter of taste, but personally i don't like these temporary variables, tap
is your fried to get rid of 'em
add support for google analytics userId
could we agree on not merging stuff that is under discussion and not reviewed completely? |
I will be more careful next time. |
See:
https://developers.google.com/analytics/devguides/collection/analyticsjs/user-id